I uninstalled the bot that automatically closed the pull request #3042

Hello @jukent! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-07-02 18:28:57 UTC

FYI the failure in appveyor is unrelated and can be ignored

@jukent You should also give yourself credit in whats-new.rst and add stuff to api.rst. I also see that you're a first-time contributor. Welcome! We really appreciate your efforts.

FYI tests are failing with E NameError: name '_rename_var_dims_helper' is not defined: https://travis-ci.org/pydata/xarray/jobs/550940568

FYI tests are failing with E NameError: name '_rename_var_dims_helper' is not defined: https://travis-ci.org/pydata/xarray/jobs/550940568

Huh not sure why that is. I can remove that function and see if it works then.

I would definitely recommend reading through our contributor's guide on how to setup a local environment for testing your code: http://xarray.pydata.org/en/stable/contributing.html

It is much easier to fix issues when you run tests locally rather than waiting on continuous integration tests (Travis-CI) to tell you that things are broken.

I would definitely recommend reading through our contributor's guide on how to setup a local environment for testing your code: http://xarray.pydata.org/en/stable/contributing.html

It is much easier to fix issues when you run tests locally rather than waiting on continuous integration tests (Travis-CI) to tell you that things are broken.

Thank you was just needing to look into this.

Checks could be more complicated, but I think this is ready to merge!

Codecov Report

Merging #3045 into master will increase coverage by 1.01%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #3045      +/-   ##
==========================================
+ Coverage   93.78%   94.79%   +1.01%     
==========================================
  Files          68       67       -1     
  Lines       13156    12892     -264     
==========================================
- Hits        12338    12221     -117     
+ Misses        818      671     -147

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aeee7f...b3232ad. Read the comment docs.

OK fixed whats-new. Sorry it gets a little confusing if there's been a release in the meantime.

This looks good to me. @max-sixty check once more and merge?

Looking at the coverage report from codecov above, it looks like the lines where you raise ValueError for non-existent keys currently don't have any test coverage. Please add unit tests that verify the right error is raised, i.e., with pytest.raises.

The code looks great now, nice work @jukent - very succinct

If we can add that small test that @shoyer references (can do it in one of the existing test, just two lines), and potentially there's a duplication in the whatsnew.

(I'm on vacation so don't wait for me to merge)

Added those tests for for the raised Value Error.

pep8speaks has a few minor style complaints to fix: #3045 (comment)

thanks @jukent !

Thanks @jukent! We're happy to have you as a contributor!